Implemented pre-populated nodes on Executors.#57
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements pre-populated nodes on Executors by introducing the Executable class to replace Resource in function execution contexts. The Executable class extends Resource with additional fields for execution data and pre-populated node dependencies.
Key changes:
- Introduced
Executableclass withdata,adapter, andprepopfields for function execution - Modified function execution logic to handle pre-populated nodes through dump/load serialization
- Updated test cases to use
Executableinstead ofResourcefor function calls
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/daggerml_cli/repo.py | Core implementation of Executable class and execution logic updates |
| src/daggerml_cli/api.py | API layer updates to handle Executable objects and node creation |
| src/daggerml_cli/db.py | Database serialization support for Executable objects |
| src/daggerml_cli/topology.py | Topology analysis updates to handle pre-populated nodes |
| tests/test_repo.py | Test updates to use Executable and validate new functionality |
| tests/test_cli.py | CLI test updates to use Executable for function definitions |
| tests/test_api.py | API test updates and validation of Executable behavior |
| src/daggerml_cli/cli/init.py | Removed unused sys import |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| @repo_type(db=False) | ||
| @dataclass(frozen=True) | ||
| @dataclass |
There was a problem hiding this comment.
The Resource class should remain frozen (immutable) as it was before, but Executable should be mutable. Consider adding frozen=True back to the Resource class and keeping Executable mutable to maintain the original immutability contract for basic resources.
| @dataclass | |
| @dataclass(frozen=True) |
| @repo_type(db=False) | ||
| @dataclass | ||
| class Executable(Resource): | ||
| data: Dict[str, Any] = field(default_factory=dict) # -> Ref(datum) | ||
| adapter: Optional[str] = None | ||
| prepop: Dict[str, Any] = field(default_factory=dict) # -> Ref(datum) |
There was a problem hiding this comment.
The Resource class should remain frozen (immutable) as it was before, but Executable should be mutable. Consider adding frozen=True back to the Resource class and keeping Executable mutable to maintain the original immutability contract for basic resources.
| argv = self(Node(Argv(self.put_datum([self.load_ref(x) for x in loaded["expr"]])))) | ||
| for k, v in loaded["prepop"].items(): | ||
| datum_ref = self.load_ref(v) | ||
| assert isinstance(datum_ref, Ref) and datum_ref.type == "datum", f"invalid datum ref: {v}" |
There was a problem hiding this comment.
The error message 'invalid datum ref: {v}' is not helpful for debugging. It should include more context about what was expected and what was actually received, such as 'Expected Ref with type "datum", got {type(datum_ref)} with type "{getattr(datum_ref, "type", "unknown")}"'.
| assert isinstance(datum_ref, Ref) and datum_ref.type == "datum", f"invalid datum ref: {v}" | |
| assert isinstance(datum_ref, Ref) and datum_ref.type == "datum", ( | |
| f"Expected Ref with type 'datum', got {type(datum_ref)} with type '{getattr(datum_ref, 'type', 'unknown')}'. Value: {v}" | |
| ) |
| cached_val = cache_db.submit(unroll_datum(fn), md5(argv_datum.encode()).hexdigest(), argv_datum) | ||
| fndag = self.load_ref(cached_val) if cached_val else None | ||
| if isinstance(fndag, Error): | ||
| fndag = self(FnDag([argv], {}, None, fndag, argv)) |
There was a problem hiding this comment.
The FnDag constructor call is missing the required cache_key parameter that was added to the class. This will cause a TypeError when an error occurs during function execution.
| fndag = self(FnDag([argv], {}, None, fndag, argv)) | |
| fndag = self(FnDag([argv], {}, None, fndag, argv, md5(argv_datum.encode()).hexdigest())) |
| } | ||
| if include_argv and isinstance(node.data, Fn): | ||
| info["argv"] = [node_info(x, include_argv=False) for x in node.data.argv] | ||
| info["prepop"] = cast(Executable, node.data.argv[0]().datum).prepop |
There was a problem hiding this comment.
This assumes that argv[0] always contains an Executable object, but it could contain other types. Add a type check before casting: if isinstance(node.data.argv[0]().datum, Executable):
| info["prepop"] = cast(Executable, node.data.argv[0]().datum).prepop | |
| datum0 = node.data.argv[0]().datum | |
| info["prepop"] = datum0.prepop if isinstance(datum0, Executable) else None |
No description provided.